Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manage all mod_remoteip parameters supported by Apache #1882

Merged
merged 4 commits into from
Feb 8, 2019
Merged

Manage all mod_remoteip parameters supported by Apache #1882

merged 4 commits into from
Feb 8, 2019

Conversation

johanfleury
Copy link
Contributor

No description provided.

@david22swan
Copy link
Member

david22swan commented Feb 6, 2019

@johanfleury Giving this a quick look, my first thought is the lack of doc updates. For a change like this we'd need to see updated documentation covering each added parameter.
I'd also like to see the different commits squashed into one, with a more descriptive message attached to it, but that's not as important.
Overall looks like a good pr though, thanks for putting in the work.

@johanfleury
Copy link
Contributor Author

johanfleury commented Feb 6, 2019

Thanks for the quick review @david22swan, I'll work on that.

Regarding the documentation I was wondering what would be the best way to write it. Should I put it in the manifest using puppet strings syntax or should I put it in the README file?

My preference goes to the former.

@david22swan
Copy link
Member

@johanfleury If you could do both it would be great, but the manifest is the more important.

@johanfleury
Copy link
Contributor Author

@david22swan I updated the PR accordingly.

Please tell me if the documentation I added is useful and well written (English is not my first language).

@david22swan
Copy link
Member

@johanfleury Your changes look great and the pr has passed cleanly through the adhoc pipeline so I am more than happy to merge it. Thank you for putting in the work and responding so promptly.
screen shot 2019-02-08 at 10 58 01 am

@david22swan david22swan merged commit 80d7676 into puppetlabs:master Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants